pr05 Typescript #8: migrate client/components/Menubar/MenubarSubmenu #3623

clairep94 · 2025-09-03T18:29:22Z

pr05 Typescript Migration 8: Migrate the client/components/Menubar/MenubarSubmenu

Context:

Had difficulty migrating/understanding MenubarSubmenu when building PR 7, so chose to open a PR without this file and to resolve it separately.
Should be reviewed after pr05 Typescript Migration #7: migrate client components #3619 is merged for clarity.

Changes:

Menubar/MenubarSubmenu
Menubar/contexts

Other:

any updates to use named exports

Notes:

I have verified that this pull request:

has no linting errors (npm run lint)
has no test errors (npm run test)
is from a uniquely-named feature branch and is up to date with the develop branch.
is descriptively named and links to an issue number, i.e. Fixes #123

…rors

…nubarTrigger

clairep94 · 2025-09-03T18:38:14Z

client/components/Menubar/MenubarSubmenu.tsx

+  MENU = MenubarListItemRole.MENU,
+  LISTBOX = MenubarListItemRole.LISTBOX,
+  TRUE = 'true'
+}


@khanniie I'm not 100% if this was the correct approach

I was a bit confused because the original file had the below:

MenubarTrigger.propTypes = { role: PropTypes.string, hasPopup: PropTypes.oneOf(['menu', 'listbox', 'true']) }; MenubarList.propTypes = { children: PropTypes.node, role: PropTypes.oneOf(['menu', 'listbox']) };

I made the assumption the menu bar list item roles and menubar trigger aria-hasPopup are associated in the relationship defined in the enums??
Should we just remove TRUE? I didn't see TRUE being used elsewhere

based on the web docs (https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Attributes/aria-haspopup#description), it looks like they're supposed to be the same!

"Make sure the role of the element that serves as the container for the popup content is a menu, listbox, tree, grid or dialog and that the value of aria-haspopup matches the role of the popup container."

in other words, my understanding is that aria-haspopup is telling the assistive device what kind of container the "trigger button" is about to open, so it should match the type of our submenu containers.

so it should be either menu or listbox, like you said -- i guess true is a valid value (docs say "The value true is the same as menu") but if we're never using it, we should remove

clairep94 · 2025-09-03T18:41:31Z

client/components/Menubar/MenubarSubmenu.tsx

+    const { isOpen, handlers } = useMenuProps(id);
+
+    // `ref` is always a button from MenubarSubmenu, so safe to cast.
+    const buttonRef = ref as React.RefObject<HTMLButtonElement>;


wasn't sure how to resolve this other than casting, but I thought was safe bc this function is only used within this file once and it's always a button ref

sgtm!! it's kind of surprising to me that typescript wouldn't be able to infer that from the React.forwardRef function params since you gave it the HTMLButtonElement type there but i trust that it doesn't work without casting haha

clairep94 · 2025-09-03T18:42:23Z

client/components/Menubar/MenubarSubmenu.tsx

  title,
-  triggerRole: customTriggerRole,
-  listRole: customListRole,
+  triggerRole = 'menuitem',


not sure if string is too loose here, or if there is a specific enum we should be using?

defer to you but we could add another enum for list item roles! since you already have one for the list itself. i think the type would be either menuitem (for the 'menu' containers) or option (for the 'listbox' containers)

clairep94 · 2025-09-03T18:43:57Z

@raclim @khanniie ready for review! this is build off of PR 7, which was big so might be best to wait till PR 7 is merged to look at this one

…ild__MenubarSubmenu_attempt_2

khanniie

thanks so much for putting this together! it was definitely a very complicated file haha but great work

khanniie · 2025-09-10T16:50:20Z

client/components/Menubar/MenubarSubmenu.tsx

+  MENU = MenubarListItemRole.MENU,
+  LISTBOX = MenubarListItemRole.LISTBOX,
+  TRUE = 'true'
+}


based on the web docs (https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Attributes/aria-haspopup#description), it looks like they're supposed to be the same!

"Make sure the role of the element that serves as the container for the popup content is a menu, listbox, tree, grid or dialog and that the value of aria-haspopup matches the role of the popup container."

in other words, my understanding is that aria-haspopup is telling the assistive device what kind of container the "trigger button" is about to open, so it should match the type of our submenu containers.

so it should be either menu or listbox, like you said -- i guess true is a valid value (docs say "The value true is the same as menu") but if we're never using it, we should remove

khanniie · 2025-09-10T17:00:16Z

client/components/Menubar/MenubarSubmenu.tsx

+    const { isOpen, handlers } = useMenuProps(id);
+
+    // `ref` is always a button from MenubarSubmenu, so safe to cast.
+    const buttonRef = ref as React.RefObject<HTMLButtonElement>;


sgtm!! it's kind of surprising to me that typescript wouldn't be able to infer that from the React.forwardRef function params since you gave it the HTMLButtonElement type there but i trust that it doesn't work without casting haha

khanniie · 2025-09-10T17:04:32Z

client/components/Menubar/MenubarSubmenu.tsx


-export function useMenuProps(id) {
+/** Custom subset of valid list item roles for the Menubar list items */
+export enum MenubarListItemRole {


small nit but it looks like these are values for the container, not the items within the container? should we update the comment and interface name?

maybe like "MenuContainerRole"

Also agree, I think it might make sense to update the name here!

khanniie · 2025-09-10T17:15:00Z

client/components/Menubar/MenubarSubmenu.tsx

  title,
-  triggerRole: customTriggerRole,
-  listRole: customListRole,
+  triggerRole = 'menuitem',


defer to you but we could add another enum for list item roles! since you already have one for the list itself. i think the type would be either menuitem (for the 'menu' containers) or option (for the 'listbox' containers)

raclim · 2025-09-11T21:24:50Z

Thanks so much for your work on this! I also think this looks great, and I really like how the refactor for the menubarsubmenu component feels more straightforward and easy to read! Can hold on merging if you'd like to make any further tweaks!

…ild__MenubarSubmenu_attempt_2

clairep94 · 2025-09-15T18:57:28Z

@raclim @khanniie ready for re-review!

khanniie

one small comment but overall LGTM! thank you so much!

khanniie · 2025-09-16T05:45:22Z

client/components/Menubar/MenubarSubmenu.tsx

 * -----------------------------------------------------------------------------------------------*/

+/** Custom subset of valid values for aria-hasPopup for the MenubarTrigger */
+enum MenubarTriggerAriaHasPopup {


this is a bit of a nit but if it looks exactly the same as menucontainerrole why not just use that instead of duplicating?

clairep94 added 8 commits September 1, 2025 20:03

MenubarSubmenu: update to tsx, no-verify

6485550

MenubarSubmenu: wip add interface --no-verify

839a8aa

merge with pr05/Migrate_client_components_final_rebuild

ff0100b

MenubarSubmenu: update context interfaces, wip, no-verify

632c11a

MenubarSubmenu: define interfaces, no-verify

d8327b5

MenubarSubmenu: update to named export, no-verify for remaining ts er…

1127755

…rors

MenubarSubmenu: cast ref prop as RefObject to resolve typeerror in Me…

913d9e6

…nubarTrigger

MenubarSubmenu: fix final typeerrors by adding htmlElement check

2427c1f

clairep94 added the pr05 Grant Projects pr05 Grant Projects label Sep 3, 2025

clairep94 changed the title ~~Pr05/migrate client components final rebuild menubar submenu attempt 2~~ pr05 Typescript #8: migrate client/components/Menubar/MenubarSubmenu Sep 3, 2025

clairep94 commented Sep 3, 2025

View reviewed changes

clairep94 marked this pull request as ready for review September 3, 2025 18:42

Merge branch 'develop' into pr05/Migrate_client_components_final_rebu…

168a9e5

…ild__MenubarSubmenu_attempt_2

khanniie previously approved these changes Sep 10, 2025

View reviewed changes

khanniie requested a review from raclim September 10, 2025 17:20

MenubarSubmenu: update to address feedback for MenuContainerRole

4e993cf

clairep94 dismissed khanniie’s stale review via 4e993cf September 15, 2025 18:52

clairep94 and others added 2 commits September 15, 2025 19:56

MenubarSubmenu: add enum for menubar list item roles

eef7fe0

Merge branch 'develop' into pr05/Migrate_client_components_final_rebu…

1f200da

…ild__MenubarSubmenu_attempt_2

khanniie approved these changes Sep 16, 2025

View reviewed changes

Uh oh!

pr05 Typescript #8: migrate client/components/Menubar/MenubarSubmenu #3623

Are you sure you want to change the base?

pr05 Typescript #8: migrate client/components/Menubar/MenubarSubmenu #3623

Uh oh!

Conversation

clairep94 commented Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

pr05 Typescript Migration 8: Migrate the client/components/Menubar/MenubarSubmenu

Context:

Changes:

Notes:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clairep94 commented Sep 3, 2025

Uh oh!

khanniie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raclim commented Sep 11, 2025

Uh oh!

clairep94 commented Sep 15, 2025

Uh oh!

khanniie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

clairep94 commented Sep 3, 2025 •

edited

Loading